Skip to content

Conversation

@TRomesh
Copy link
Contributor

@TRomesh TRomesh commented Nov 24, 2025

  • DAM-enabled GraphQL queries: Content references now fetch asset details (images, videos, files) with renditions, focal points, and tags when damEnabled param is set to true.

  • Conditional fragment generation: DAM fragments only included when needed to optimize query size.

  • Enhanced ContentReferenceItem: Added BaseAsset type and properly typed PublicImageAsset, PublicVideoAsset, PublicRawFileAsset with discriminated unions.

  • Updated InferredContentReference: Now includes optional item property with asset details.

  • Added helper functions to getPreviewUtils(): Extracts URL from assets, supports rendition selection, auto-appends preview tokens.

    • src: Update method to take contentReference type object and extract url or Url from DAM.
    • srcset: Generates responsive srcset from renditions with automatic width deduplication.
    • alt: Extracts AltText from DAM assets.

@TRomesh TRomesh requested a review from Copilot November 24, 2025 11:32
Copilot finished reviewing on behalf of TRomesh November 24, 2025 11:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds comprehensive support for Content Management Platform (CMP) Digital Asset Management (DAM) assets to the Optimizely CMS SDK. It enables applications to fetch and utilize rich asset metadata including renditions, focal points, and tags from the GraphQL API when working with content references.

Key Changes

  • Conditional DAM fragment generation: Introduces a damEnabled flag to GraphClient that conditionally includes DAM-specific GraphQL fragments only when needed, optimizing query size for non-DAM scenarios
  • Enhanced type system: Adds strongly-typed asset models (PublicImageAsset, PublicVideoAsset, PublicRawFileAsset) with discriminated unions and updates InferredContentReference to include optional asset item data
  • Helper utilities: Extends getPreviewUtils() with src(), srcset(), and alt() functions that intelligently extract URLs and metadata from DAM assets with automatic preview token handling and rendition selection

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/optimizely-cms-sdk/src/model/assets.ts Defines TypeScript types for DAM assets (PublicImageAsset, PublicVideoAsset, PublicRawFileAsset) with renditions, tags, and metadata
packages/optimizely-cms-sdk/src/infer.ts Updates ContentReferenceProperty inference to include optional item field with asset details
packages/optimizely-cms-sdk/src/util/baseTypeUtil.ts Adds CONTENT_REFERENCE_ITEM_FRAGMENTS and updates buildBaseTypeFragments() to conditionally include DAM fragments
packages/optimizely-cms-sdk/src/graph/createQuery.ts Propagates damEnabled flag through all query generation functions to conditionally include ContentReferenceItem fragments
packages/optimizely-cms-sdk/src/graph/index.ts Adds damEnabled option to GraphClient constructor and passes it to query generation
packages/optimizely-cms-sdk/src/react/server.tsx Enhances getPreviewUtils() with src(), srcset(), and alt() helpers supporting both legacy URLs and DAM assets
packages/optimizely-cms-sdk/src/react/__test__/getPreviewUtils.test.tsx Comprehensive test coverage for new helper functions including rendition selection, deduplication, and preview token handling
packages/optimizely-cms-sdk/src/graph/__test__/createQueryDAM.test.ts Tests DAM fragment generation across simple, nested, and array scenarios with damEnabled flag
samples/nextjs-template/src/components/AboutUs.tsx Demonstrates DAM usage by replacing Next.js Image component with native img using src() and alt() helpers
samples/nextjs-template/src/components/SmallFeature.tsx Updates src() call to use new ContentReference API (passes entire object instead of url.default)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@TRomesh TRomesh had a problem deploying to Vercel preview environment November 24, 2025 14:09 — with GitHub Actions Failure
@TRomesh TRomesh marked this pull request as ready for review November 24, 2025 14:12
Copy link
Contributor

@exacs exacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks good! I have no objections in the GraphClient and infer part.


However, I think the src, alt and srcset are too high level, especially because they are under getPreviewUtils. I expect those functions to only append preview tokens, not converting Graph responses to HTML attributes.

If you want to implement high level functions that convert graph responses into HTML attributes, they are separate functions. You can also build a React component Image (like Next.js Image) where you pass the image object (opti.image) and renders the image, but it feels a separate PR.

Comment on lines 464 to 476
alt(
input: InferredContentReference | null | undefined,
fallback?: string
): string {
if (!input) return fallback ?? '';

// Check if item has AltText property (PublicImageAsset or PublicVideoAsset)
if (input.item && 'AltText' in input.item) {
const altText = input.item.AltText ?? '';
return altText || (fallback ?? '');
}
return url;

return fallback ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this alt function should not belong here (getPreviewUtils) because it has nothing to do with preview

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move this outside of getPreviewUtils.

Comment on lines 417 to 440
srcset(input: InferredContentReference | null | undefined): string {
if (!input?.item || !('Renditions' in input.item)) return '';

const renditions = input.item.Renditions;
if (!renditions || renditions.length === 0) return '';

// Track seen widths to avoid duplicate width descriptors
const seenWidths = new Set<number>();

const srcsetEntries = renditions
.filter((r) => {
if (!r.Url || !r.Width) return false;
// Skip if we've already seen this width
if (seenWidths.has(r.Width)) return false;
seenWidths.add(r.Width);
return true;
})
.map((r) => {
const url = appendPreviewToken(r.Url!);
return `${url} ${r.Width}w`;
});

return srcsetEntries.join(', ');
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function does too many things. If this is under getPreviewUtils, it should not convert to the HTML attribute srcset. I think is better if this returns InferredContentReference with the preview token appended for each URL and nothing more.

And then, separately, outside getPreviewUtils, we can implement something that converts InferredContentReference to srcset

Actually, if the function only appends preview tokens to URLs, we don't need two separate functions (src and srcset). I think is more interesting to have one function that does that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like i missed a part this function should be able fetch a particular rendition. But anyway I will move it outside

Comment on lines 364 to 367
src(
input: string | InferredContentReference | null | undefined,
options?: { renditionName?: string }
): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have concerns here. Since this src function is inside getPreviewUtils, I think the function should be much lower level and only append preview tokens to image URLs

I mean:

  • if input is a string, it returns the url with the preview token as string
  • if input is InferredContentReference, returns InferredContentReference where each url has the preview token

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we return InferredContentReference then a user will have to handle a lot of stuff like validations and guards. I'll create something similar to getPreviewUtils which focus on damAssets . It should reduce number of steps to get srcset, alt.

@TRomesh TRomesh had a problem deploying to Vercel preview environment November 25, 2025 15:38 — with GitHub Actions Failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants